Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to override the file name "rerun.txt" #21

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dbrock
Copy link

@dbrock dbrock commented Feb 15, 2015

Especially when running in a read-only filesystem, one needs to be able to configure the file name used for the rerun.txt file.

@e2
Copy link

e2 commented Feb 17, 2015

It's a bit tricky to pass options to a formatter (guard-rspec has the same issues).

Still I'd prefer the following:

  1. some documentation in the README (because otherwise people won't find out about this, except maybe for release notes)
  2. a better name than 'RERUN_TXT', e.g. 'RERUN_FILE'
  3. namespacing the variable, e.g. GUARD_CUCUMBER_RERUN_FILE
  4. you'd need to tell cucumber to use (and maybe set) that env variable: https://github.com/guard/guard-cucumber/blob/master/lib/guard/cucumber.rb#L116-L118
  5. (optional) tests would be nice - especially for edge cases (e.g. blank text)
  6. I would be nice if it was configurable from the Guardfile instead
  7. You could use Nenv to make namespacing and testing easier, e.g.:
# initalization
@env = Nenv(:guard_cucumber) { |n| n.create_method(:rerun_file) { |name| name || "rerun.txt"  } }
# usage
File.open(@env.rerun_file, "w") # etc

@dbrock dbrock changed the title Parameterize "rerun.txt" on $RERUN_TXT Make it possible to override the file name "rerun.txt" Mar 27, 2015
@dbrock
Copy link
Author

dbrock commented Mar 27, 2015

Done:

  • some documentation in the README
  • a better name than RERUN_TXT, e.g. RERUN_FILE
  • namespacing the variable, e.g. GUARD_CUCUMBER_RERUN_FILE
  • you'd need to tell Cucumber to use (and maybe set) that env variable

Not sure why we would need to set it, but I changed it to read it anyway.

  • tests would be nice (optional)

Added tests for both reading and writing the file. Could probably refactor the read-side tests to use this new mechanism as the main path and then a simpler test to verify that it defaults to rerun.txt, but I think the current code is OK.

Not done:

  • especially for edge cases (e.g. blank text)

I actually figured this was less important than testing the main path.

  • I would be nice if it was configurable from the Guardfile instead

Didn't look into this, but I agree that it would probably be nice.

  • You could use Nenv to make namespacing and testing easier

Didn't have time to look into this, but thanks for the pointer.

Sorry about including the Cucumber 2.0 fix in this pull request, hope that's OK.

@e2
Copy link

e2 commented Mar 27, 2015

Don't worry about Cucumber 2.0, since that has to be released first anyway. I think there's no reason for people not to switch to cucumber 2.0 (since it's supposed to be possibly 100% compatible) - so this comes later.

A few notes about improving the code you based the changes on:

  1. Instead of File.exist? and File.open and File.read - replace that with IO.read and exception handling - and use Pathname instead, e.g.
errors_file = Pathname(rerun_file)
begin
  errors_file.read.split(" ")
  errors_file.unlink
rescue Errno::ENOENT
end

This should make tests easier, because you're only stubbing 1 method instead of 3.
Pathname#read calls IO.read (so stub that instead), and Pathname#unlink calls File.unlink (so stub that instead).

  1. Remove the environment variable from the "main" guard-cucumber code (and the README). Instead, add a proper option in the Guardfile, e.g.
guard :cucumber, rerun_file: 'my_rerun.txt' do
# (...)
end

Then, in runner.rb - set the environment variable in the system() call (first argument of system is hash of environment variables). This will run cucumber with the environment variable, and so the notifier will know where to write the failures - meanwhile the user doesn't need to know there's even an environment variable under the hood.

This way, you can make the notifier always expect the environment variable (it should even be so that it fails without it - less code, less tests).

Let me know if you have questions.

@e2
Copy link

e2 commented Mar 29, 2015

You can rebase to master branch (for Cucumber 2.x).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants